Could you move this arg below the constants. In this codebase, the conventional structure is class constants, then variables, then functions

You do not need to move this check to process_report.py. We only want to move the fetching of the env var. Error checking can stay in the invoice class for now.

You should add CHROME_BIN_PATH in REQUIRED_ENV_VARS

There's no reason to perform validation in two different places. If we're using pydantic_settings to manage environment variables, then rather than using required_env_files here, just mark chrome_bin_path as required in process_report.settings.Settings. Currently it is optional:

    chrome_bin_path: str | None = None

To make it required:

    chrome_bin_path: str

This will cause pydantic to throw a validation error when instantiating Settings:

pydantic_core._pydantic_core.ValidationError: 1 validation error for Settings
chrome_bin_path
  Field required [type=missing, input_value={}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.10/v/missing

Similarly, you can remove the check for KEYCLOAK_CLIENT_ID and KEYCLOAK_CLIENT_SECRET here by adding a validator to Settings:

from pydantic import model_validator
from pydantic_settings import BaseSettings

class Settings(BaseSettings):
    ...

    
    @model_validator(mode="after")
    def check_keycloak_auth(self):
        if not self.coldfront_api_filepath and not (
            self.keycloak_client_id and self.keycloak_client_secret
        ):
            raise ValueError(
                "You must either set coldfront_api_filepath or provide keycloak credentials in "
                "KEYCLOAK_CLIENT_ID and KEYCLOAK_CLIENT_SECRET"
            )

You probably want to catch the exception from pydantic to produce a more friendly error message that does not include a full Python traceback.

Thank you for the very detailed feedback. I don't know why I didn't thought of getting rid of that hacky function in the first place. Your feedback makes it look obvious

Related to the adding CHROME_BIN_PATH to REQUIRED_ENV_VARS, you should have the env var fetched after the env vars have been validated

What was the motivation for this change? I think it's generally better practice to read your environment variables early, rather than doing it inline like this.

I've written so that the env var is loaded by the settings module

Why are we setting CHROME_BIN_PATH to an invalid path here?

The unit tests were designed to not require integration with 3rd-party tools. We do have an actual integration test for Chrome that installs Chromium and sets a real path.

As for why the env var is set, rather than not set at all, I wanted to check that code actual read the env var CHROME_BIN_PATH, whatever it maybe.

I don't really like this. Is there any way you can provide these values through mocking during the tests?

To echo what Kristi said: unit tests should never depend on the value of external environment variables. Regardless of whether CHROME_BIN_PATH is set to an invalid value, a valid path, or is unset, the unit tests should behave the same.

You don't even need to use mocking; you can just set environment variables directly in your unit tests

@larsks @knikolla Because the pydantic settings fetched the environment variables at import time, I don't believe mocking the env vars within the tests won't solve it. Something like this:

@mock.patch.dict(os.environ, {"CHROME_BIN_PATH": "/usr/foo/chromium"})
class TestInvoice(TestCase):

Or...

def setUp():
  monkeypatch.setenv("CHROME_BIN_PATH", "bar")

Still raises a validation error from Pydantic.

The solutions I found were to use a tool like pytest-env that sets environment variables before import time, to import the settings module (or the modules that use it) in setUp() rather at the top-level of the file, or refactor settings.py so it doesn't fetch env vars at import time, which has bigger upfront work. What are you thoughts?

You should be able to work around this by setting your environment variables before anything tries to import your settings module:

diff --git a/process_report/tests/integration/test_chromium.py b/process_report/tests/integration/test_chromium.py
index 2e957a8..4bb1995 100644
--- a/process_report/tests/integration/test_chromium.py
+++ b/process_report/tests/integration/test_chromium.py
@@ -3,6 +3,9 @@ from typing import override
 
 import pandas as pd
 
+os.environ["CHROME_BIN_PATH"] = "/foo"
+os.environ["COLDFRONT_API_FILEPATH"] = "/bar"
+
 from process_report.invoices.pi_specific_invoice import PIInvoice
 from process_report.invoices import invoice
 from process_report.tests.base import BaseTestCaseWithTempDir

We have talked before about why it's a good to avoid code that runs at import time, and this is a perfect example of how that complicates testing.

This is a bit janky and would require changes across all of the test files.

I think using pytest-env would be the best short-term fix.

There's no reason to perform validation in two different places. If we're using pydantic_settings to manage environment variables, then rather than using required_env_files here, just mark chrome_bin_path as required in process_report.settings.Settings. Currently it is optional:

    chrome_bin_path: str | None = None

To make it required:

    chrome_bin_path: str

This will cause pydantic to throw a validation error when instantiating Settings:

pydantic_core._pydantic_core.ValidationError: 1 validation error for Settings
chrome_bin_path
  Field required [type=missing, input_value={}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.10/v/missing

Similarly, you can remove the check for KEYCLOAK_CLIENT_ID and KEYCLOAK_CLIENT_SECRET here by adding a validator to Settings:

from pydantic import model_validator
from pydantic_settings import BaseSettings

class Settings(BaseSettings):
    ...

    
    @model_validator(mode="after")
    def check_keycloak_auth(self):
        if not self.coldfront_api_filepath and not (
            self.keycloak_client_id and self.keycloak_client_secret
        ):
            raise ValueError(
                "You must either set coldfront_api_filepath or provide keycloak credentials in "
                "KEYCLOAK_CLIENT_ID and KEYCLOAK_CLIENT_SECRET"
            )

You probably want to catch the exception from pydantic to produce a more friendly error message that does not include a full Python traceback.

I think it makes sense to keep providing a default as before as you've now made the environment variable required for running all unit tests for no obvious gain.

I don't really like this. Is there any way you can provide these values through mocking during the tests?